Vtgate refactors. #3539
Vtgate refactors. #3539
Conversation
* Remove unused 'method string' from Executor.execute(). * Remove srv_topo_timeout flag from resilient_srv_topo_server. We can just used the provided context, no need to slap on a random timeout. * Update MessageStream comment, we do support multiple shards. * In vtgate SplitQuery, use closures directly, inside of wrapping them in a method.
The new package will also eventually contain: * the ResilientSrvTopoServer implementation of that interface. * helper methods to resolve cell/keyspace/shards.
It's more consistent to have the watch code in srvtopo.
* SrvVSchema update was not thread-safe. Now it is protected by the mutex. * Added a unit test for srvtopo.Server.WatchSrvVSchema.
|
@sougou I think this is a good stand-alone PR now, ready for review. We can talk about the next steps. |
There was a problem hiding this comment.
@alainjobart This refactor looks good to me.
Thanks for cleaning up the stray method argument to execute (from an earlier version of the logStats changes) and the need for a lock on e.srvVschema (which I noticed in the Vindex DDL PR as well).
Speaking of which -- I had started down somewhat related changes as part of #3498.
Specifically, in order to update vindex definitions from the vtgate, I added some methods fo the SrvTopoServer interface in 8fcc136 and pulled out some of the vschema management logic from the executor into a VindexManager class in 015090f.
Both of those will obviously need to be reworked in light of these changes, but more generally I wanted to get your thoughts on both of these as it relates to this work overall.
|
@demmer I had missed the work you were doing in the related areas, but I think it's somewhat orthogonal. I like your VSchemaManager, it's a good way to group all the related VSchema functions. My srvtopo package cannot contain the VSchemaManager, as I don't really want it to depend on vtgate/vindexes. So it stops at the proto layer (vschemapb.SrvVSchema), and vtgate needs to do the rest. If anything, I think this change will simplify a chunk of your work, just register the VSchemaManager as the listener of the WatchSrvVSchema. |
A 'not found' is not a vschema watch error any more.
|
@demmer I missed one part, on adding new methods to SrvTopoServer to save the VSchema. I think this feels a bit out of place there. Instead, how about using the *topo.Server for this purpose? So just changing NewResolver to also take the *topo.Server, save it in the Resolver object, and use that guy to change the VSchema. I think that's more in line with the rest. |
|
@alainjobart I considered that as well and frankly that was what I was looking for some guidance / opinions on... From my read of the code, the narrower SrvTopoServer interface seemed to be intended to describe the methods called from the vtgate tier as opposed to the vtctld tier. As such it seemed better to add the requisite methods to that interface as opposed to passing the full topo.Server all the way down. As a concrete consequence of this, the sandboxTopo doesn't implement the full topo.Server contract, and if we went with this approach we would have to either make it do that, or change the vtgate tests so it uses the memorytopo. I'm happy to go either way here but am curious to hear your thoughts. |
|
I see the SrvTopoServer as different from topo.Server now: it's the interface you use for serving queries. It should be read-only, and just provide a useful view of the topo (and maybe cache it). So maybe just forward the *topo.Server inside NewResolver to the VSchemaManager, and Resolver can just hang on to the SrvTopoServer... |
|
OK let me think about that - another option would be to add an new interface (e.g. |
Starting to implement pieces of doc/TabletRouting.md.
The first part here is to create a srvtopo module that can contain the new logic.